Stop the test suite writing into its own repository (#431) - #440
Conversation
tests/scenarios/test_network_failures.py generated pipeline YAML that
embedded whichever ephemeral port its mock server happened to bind, then
wrote it into tests/scenarios/test_pipelines/ -- a tracked directory. So
every run left the working tree dirty with a different random port, and
whichever port was committed last became the fixture everyone else got.
Observed three times in one session; the diff is always the same two
files with a new port number.
The directory was also spelled as an absolute path to one developer's
home directory:
Path("/Users/jmanning/orchestrator/tests/scenarios/test_pipelines")
On any other machine `mkdir(exist_ok=True)` raises FileNotFoundError on
the missing parent, so both classes errored out rather than ran. Three
more absolute paths did the same elsewhere: two `sys.path.insert` calls,
and an examples directory whose absence made every real-world example
test `pytest.skip` silently on any machine but one.
All six YAML files in tests/scenarios/test_pipelines/ were written by
this test module and read by nothing else -- outputs masquerading as
fixtures -- so they are deleted rather than relocated.
The paths are now repo-relative, and generated pipelines go to pytest's
`tmp_path` via a small `_UsesTemporaryPipelineDir` mixin shared by the
two classes that need it.
CI gains a guard: after the legacy suite runs, `git diff --quiet` must
hold. Deliberately NOT continue-on-error -- the backlog is allowed to be
red, but it is not allowed to modify tracked files. Without the guard
this regresses silently the next time someone writes a test that saves
output "somewhere convenient".
While in that job, `-rN` becomes `-rfE`. `-rN` suppresses the summary
entirely, so the job publishes a single scalar and "did anything
regress?" cannot be answered from CI output -- attributing a change
meant re-running both sides locally. `-rfE` names the failures and
errors, making the backlog diffable run to run. The tally the summary
step reads is still the last line, so that step is unaffected.
Verified by A/B on the same 8 tests:
main: 8 failed, 2 tracked files modified
this: 8 failed, working tree clean
The 8 failures are the pre-existing "no models available" backlog and
are unchanged; only the mutation is gone.
Blocking suite 456 passed, 13 skipped, 0 failed. Collection 3251,
unchanged. ruff clean on src and on every file touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
CI 9/9 green, and the legacy numbers reconcile exactly — but only after I fixed my own comparison.
Six tests moved The remaining two of the eight in that file were already The mistake worth recordingMy first pass at this compared against run PR-branch runs and post-merge runs are different commits with different content. Only post-merge Byte-identical across two consecutive runs, so a ±2 swing would have been real — which is exactly why I chased it. The guard ranThe
|
1 participant
Closes #431.
What was happening
tests/scenarios/test_network_failures.pygenerates pipeline YAML embedding whichever ephemeral port its mock server happened to bind, then wrote it intotests/scenarios/test_pipelines/— a tracked directory. Every run left a diff:Whichever port was committed last became the fixture everyone else got. Observed three separate times in one working session.
The part that wasn't in the issue
The directory was spelled as an absolute path to one developer's home directory:
mkdir(exist_ok=True)withoutparents=TrueraisesFileNotFoundErrorwhen the parent is missing — so on any other machine, including CI, both classes in that file errored out rather than ran. Some of the legacy job's 195 errors are this.Three more absolute paths did the same elsewhere:
test_control_flow_conditional.py:11sys.path.insertto a nonexistent dirtest_control_flow_dynamic.py:13scenarios/test_real_world_examples.py:34examples_dirmissing → every example silentlypytest.skippedThat last one is the worst of the three: it doesn't fail, it quietly reports nothing to test.
The fix
Path(__file__).resolve().parents[...]).tmp_path, via a small_UsesTemporaryPipelineDirmixin shared by the two classes that need it — no duplicated setup/teardown.tests/scenarios/test_pipelines/are deleted. Every one was written by this module and read by nothing else: outputs masquerading as fixtures. Verified by grep before removing.CI guard
The fix alone regresses the next time someone saves test output "somewhere convenient", so the legacy job now asserts
git diff --quietafterwards. Deliberately notcontinue-on-error— the backlog is allowed to be red, it is not allowed to modify tracked files.One change you may want to veto
While in that job,
-rN→-rfE.-rNsuppresses the summary entirely, so the job publishes a single scalar. That means "did anything regress?" is unanswerable from CI: a net −2 is equally consistent with 5 fixed and 3 broken. I hit this twice today and had to re-run both sides locally to attribute changes — and I reported "nothing regressed" on #438 on aggregate evidence that didn't actually support it.-rfEnames failures and errors so the backlog is diffable run to run.The summary step reads the last line for its tally, which
-rfEleaves in place, so that step is unaffected. Output gets longer — say the word and I'll drop this hunk.Verification
A/B on the same 8 tests, same environment:
mainThe 8 failures are the pre-existing "no control system provided and no models available" backlog — identical both ways. Only the mutation is gone; this PR neither fixes nor worsens those.
Blocking suite 456 passed, 13 skipped, 0 failed. Collection 3251, unchanged.
uv lock --check, ruff onsrcand on every file touched, andci.ymlYAML parse all clean.